Use correct WebSocket error codes #1753

Kludex · 2022-11-01T07:47:47Z

This is a very confusing subject.

There are 4 ways for us to send websocket.disconnect to the application:

WSProtocol	WebSocketProtocol	Event	Code
✅	✅	Server shutdown	`1012` (Service Restart)
✅	✅	Client sent close frame	Send the code passed to the server i.e. `wsproto` sends `event.code`, and `websockets` sends `exc.code`
✅	✅	Handshake failed	`1006` (Abnormal Closure)
✅	✅	Connection lost without close frame	`1005` (No Status Rcvd)

Note: If the connection was lost before the handshake was completed, then it should be a 1006.

This PR aims to match the behavior on both websockets implementations, and fixes https://github.com/encode/uvicorn/pull/1737/files#r1009855193.

iudeen · 2022-11-01T08:38:30Z

1012 means server restarted and indicates client that it can retry connection right? What if the server is gone away completely? Shouldn't it be 1001?🤔

Kludex · 2022-11-01T11:39:16Z

tests/protocols/test_websocket.py

@@ -553,22 +553,22 @@ async def app(scope, receive, send):
 @pytest.mark.anyio
 @pytest.mark.parametrize("ws_protocol_cls", WS_PROTOCOLS)
 @pytest.mark.parametrize("http_protocol_cls", HTTP_PROTOCOLS)
-async def test_not_accept_on_connection_lost(ws_protocol_cls, http_protocol_cls):
+async def test_connection_lost_before_handshake_complete(


A more meaningful name on what scenario we are testing.

A more meaningful name on what scenario we are testing.

ETS TRADEX SERVICES LLP
BeNADABURAMbVADAKARA
NADAPURAM , VADAKARA, Kerala Waterm98562820968
Server:4
10:10AM
366/1
02-21-2023
1/10379
SALE
210000083
403226
CardXXXXXXXXXXXXXXXXmber
CardPresent to Remove Card EntrySwipe/Chomark
APPROVAL:3BC02
1 for 192.168.89.191Rs 210000083.00
GSTs 16926006.69
GSRs 12705005.02
AMRs 239631094.71Mem to Remove Watermark
†TIP
=TOTAL
TIP SUGGESTION:
Tip 15%
Tip 18%
Tip20%
R$31500012.45 Rs37800014.94 Rs4200C

Kludex · 2022-11-01T11:39:45Z

tests/protocols/test_websocket.py

@@ -577,6 +577,8 @@ async def websocket_session(uri):
        task.cancel()
        send_accept_task.set()

+    assert disconnect_message == {"type": "websocket.disconnect", "code": 1006}


The connection was closed without a close frame, so we send a 1006.

Kludex · 2022-11-01T11:40:54Z

tests/protocols/test_websocket.py


    assert frames == [b"abc", b"abc", b"abc"]
+    assert disconnect_message == {"type": "websocket.disconnect", "code": 1000}


The client closed the connection with a close frame, and the code interpreted by the WebSocket packages is 1000 i.e. normal closure.

Kludex · 2022-11-01T11:43:02Z

tests/protocols/test_websocket.py

-        async with websockets.client.connect(uri):
-            while True:
-                await asyncio.sleep(0.1)
+        await websockets.client.connect(uri)


This was changed because connect hangs until we are able to connect with the server. There was a previous misconception that we'd actually hit the while True logic.

We are never able to complete the handshake, as we don't send the websocket.accept, so we cancel the task below and let the application continue.

Kludex · 2022-11-01T11:45:27Z

uvicorn/protocols/websockets/websockets_impl.py

+        self.lost_connection_before_handshake = (
+            not self.handshake_completed_event.is_set()
+        )


We need to know if the connection was lost before the handshake was completed to determine what code we'll be sending to the application.

If we lost connection before the handshake is completed, then we should send a 1006.

If we lost connection after the handshake is completed, then we should send a 1005.

Kludex · 2022-11-01T12:10:16Z

uvicorn/protocols/websockets/websockets_impl.py

@@ -335,11 +339,13 @@ async def asgi_receive(

        await self.handshake_completed_event.wait()

-        if self.closed_event.is_set():
-            # If client disconnected, use WebSocketServerProtocol.close_code property.
+        if self.lost_connection_before_handshake:


Instead of lost_connection_before_handshake, I can create an attribute called lost_connection, and do the conditional before the await self.handshake_completed_event.wait() above. What do you prefer? 🤔

(I think this alternative will work, I'm not sure if I'm missing something...)

Meaning we wouldn’t hit await self.handshake_completed_event.wait() at all in connection is lost?

If that’s the case, it seems better 🤔

What I said doesn't work.

Kludex · 2022-11-01T12:18:07Z

1012 means server restarted and indicates client that it can retry connection right? What if the server is gone away completely? Shouldn't it be 1001?thinking

Conceptually, if we think about a web server, I think it doesn't make sense to send a code that says that the server has stopped for good. We think about it as a long-living software.

In other words, when we call "shutdown", it's because it's going to be up again.

If the application wants, it can send a websocket.close with 1001, and shutdown the server. 🤷‍♂️

iudeen · 2022-11-01T12:19:33Z

In other words, when we call "shutdown", it's because it's going to be up again.

Thanks! This justifies!

Kludex · 2022-11-17T15:06:06Z

Maybe this is not trivial, but I really want to include this in the release. Mainly because this is the "WebSockets release". 👀

iudeen

This looks good!😄

It'd be great if someone can confirm the comment on lost_connection_before_handshake change where Kludex asked about an alternative approach 😊

Kludex · 2022-11-20T12:22:08Z

uvicorn/protocols/websockets/wsproto_impl.py

        self.connections.remove(self)

        if self.logger.level <= TRACE_LOG_LEVEL:
            prefix = "%s:%d - " % tuple(self.client) if self.client else ""
            self.logger.log(TRACE_LOG_LEVEL, "%sWebSocket connection lost", prefix)

+        self.handshake_complete = True


This is matching the behavior with the websockets implementation.

Kludex marked this pull request as draft November 1, 2022 07:49

Use correct WebSocket error codes

71b0541

Kludex force-pushed the map-ws-error-codes branch from 3e01fcd to 71b0541 Compare November 1, 2022 07:50

Kludex mentioned this pull request Nov 1, 2022

[possible bug] 'code' is not labeled as optional django/asgiref#340

Closed

Fix all codes

91b0633

Kludex force-pushed the map-ws-error-codes branch from 1a1a0b8 to 91b0633 Compare November 1, 2022 08:39

Remove uncovered lines on tests

e562633

Kludex force-pushed the map-ws-error-codes branch from 04277bd to cec0a7d Compare November 1, 2022 11:51

Kludex marked this pull request as ready for review November 1, 2022 11:56

Check the disconncet code

3f91b00

Kludex force-pushed the map-ws-error-codes branch from cec0a7d to 3f91b00 Compare November 1, 2022 11:56

Kludex commented Nov 1, 2022

View reviewed changes

Kludex requested a review from tomchristie November 1, 2022 12:10

Kludex mentioned this pull request Nov 1, 2022

Version 0.20.0 #1732

Merged

13 tasks

Kludex mentioned this pull request Nov 1, 2022

Add test to simulate lost connection on server #758

Closed

Kludex requested review from a team and euri10 November 3, 2022 21:12

Kludex added need review websockets labels Nov 10, 2022

Kludex added this to the Version 0.20.0 milestone Nov 10, 2022

Kludex requested a review from florimondmanca November 14, 2022 12:52

iudeen approved these changes Nov 17, 2022

View reviewed changes

Kludex commented Nov 20, 2022

View reviewed changes

Kludex merged commit 41156aa into master Nov 20, 2022

Kludex deleted the map-ws-error-codes branch November 20, 2022 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use correct WebSocket error codes #1753

Use correct WebSocket error codes #1753

Kludex commented Nov 1, 2022 •

edited

Loading

iudeen commented Nov 1, 2022 •

edited

Loading

Kludex Nov 1, 2022

abey369 Feb 22, 2023

Kludex Nov 1, 2022

Kludex Nov 1, 2022

Kludex Nov 1, 2022

Kludex Nov 1, 2022

Kludex Nov 1, 2022

iudeen Nov 4, 2022

Kludex Nov 20, 2022

Kludex commented Nov 1, 2022

iudeen commented Nov 1, 2022

Kludex commented Nov 17, 2022

iudeen left a comment

Kludex Nov 20, 2022


		assert frames == [b"abc", b"abc", b"abc"]
		assert disconnect_message == {"type": "websocket.disconnect", "code": 1000}

Use correct WebSocket error codes #1753

Use correct WebSocket error codes #1753

Conversation

Kludex commented Nov 1, 2022 • edited Loading

iudeen commented Nov 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kludex commented Nov 1, 2022

iudeen commented Nov 1, 2022

Kludex commented Nov 17, 2022

iudeen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kludex commented Nov 1, 2022 •

edited

Loading

iudeen commented Nov 1, 2022 •

edited

Loading